feat(error): display colored error labels on TTY stderr#466
feat(error): display colored error labels on TTY stderr#466anshul-garg27 wants to merge 3 commits intogoogleworkspace:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for error reporting by introducing colored, human-readable labels to stderr output when it's an interactive terminal. This change allows users to quickly grasp the nature of an error without needing to parse the full JSON output, while ensuring that machine-readable JSON on stdout remains untouched. The solution is lightweight, relying on standard Rust libraries and ANSI escape codes, and respects common environmental variables for color preferences. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🦋 Changeset detectedLatest commit: 5efacf5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Code Review
This pull request introduces a great usability enhancement by adding colored error labels for TTY output, while correctly respecting the NO_COLOR standard and keeping machine-readable output on stdout clean. The implementation is well-structured. I have one suggestion to improve test reliability by preventing potential race conditions when tests are run in parallel.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces colored error labels on stderr for TTY environments, which is a great improvement for user experience. The implementation correctly detects TTYs and respects the NO_COLOR standard. The changes also include relevant tests. I have one suggestion to make the new error summary conditional to avoid a potentially breaking change for non-interactive use cases.
|
Ready for review — all tests passing, Gemini feedback addressed. @jpoehnelt |
|
/gemini review |
Closes googleworkspace#317. When stderr is connected to a terminal, error output now includes a colored label before the JSON dump: error[api]: Not Found error[auth]: Token expired error[validation]: missing arg (yellow for non-fatal) error[discovery]: fetch failed - API, Auth, Discovery, Other errors: bold red - Validation errors: bold yellow (often user-correctable) - accessNotConfigured hint: bold cyan Respects NO_COLOR env var (https://no-color.org/). JSON output on stdout is never colored (machine-readable). No new dependencies — uses std::io::IsTerminal (stable since Rust 1.70) and raw ANSI escape codes.
Address Gemini review: the test_colorize_respects_no_color_env test modifies a global env var (NO_COLOR) which can race with parallel tests. Add serial_test::serial attribute for safe sequential execution.
5efacf5 to
96ae271
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces colored error labels for TTY stderr output, which is a great enhancement for user experience. The implementation correctly checks for TTY and respects the NO_COLOR environment variable. The code is clean and includes relevant tests. I have one suggestion to improve the robustness of a test case to prevent potential flakiness.
| std::env::set_var("NO_COLOR", "1"); | ||
| let result = colorize("hello", "31"); | ||
| std::env::remove_var("NO_COLOR"); | ||
| assert_eq!(result, "hello"); |
There was a problem hiding this comment.
This test modifies a global environment variable, which can lead to flaky tests if not handled carefully. If colorize() panics, std::env::remove_var("NO_COLOR") will not be called, leaving the environment variable set for subsequent tests. Using std::panic::catch_unwind ensures that the cleanup code is executed even if the tested function panics, making the test more robust.
| std::env::set_var("NO_COLOR", "1"); | |
| let result = colorize("hello", "31"); | |
| std::env::remove_var("NO_COLOR"); | |
| assert_eq!(result, "hello"); | |
| std::env::set_var("NO_COLOR", "1"); | |
| let result = std::panic::catch_unwind(|| colorize("hello", "31")); | |
| std::env::remove_var("NO_COLOR"); | |
| assert_eq!(result.expect("colorize panicked"), "hello"); |
There was a problem hiding this comment.
yeah good point, though in practice colorize() is a simple string format that won't panic. the test also runs in a single thread with serial_test so no parallel interference. catch_unwind feels like overkill here but happy to add if the maintainer wants it.
There was a problem hiding this comment.
Code Review
This pull request introduces colored error labels for TTY stderr output, which is a great enhancement for user experience. The implementation is clean, using standard library features and respecting the NO_COLOR environment variable. The changes are well-contained within the error.rs module and are accompanied by relevant tests. I have one suggestion to improve the robustness of a test that modifies environment variables.
| std::env::set_var("NO_COLOR", "1"); | ||
| let result = colorize("hello", "31"); | ||
| std::env::remove_var("NO_COLOR"); | ||
| assert_eq!(result, "hello"); |
There was a problem hiding this comment.
This test modifies a global environment variable, which can lead to flaky tests if not handled carefully. If the call to colorize() were to panic, std::env::remove_var("NO_COLOR") would not be executed, potentially causing other tests to fail unexpectedly.
To make this test more robust, you can wrap the fallible operation in std::panic::catch_unwind. This ensures that the environment variable is cleaned up regardless of whether the tested function panics.
| std::env::set_var("NO_COLOR", "1"); | |
| let result = colorize("hello", "31"); | |
| std::env::remove_var("NO_COLOR"); | |
| assert_eq!(result, "hello"); | |
| std::env::set_var("NO_COLOR", "1"); | |
| let result = std::panic::catch_unwind(|| colorize("hello", "31")); | |
| std::env::remove_var("NO_COLOR"); | |
| assert_eq!(result.expect("colorize() panicked"), "hello"); |
There was a problem hiding this comment.
same as above — colorize is trivial and won't panic, but can add the guard if preferred
Closes #317.
Summary
When stderr is a TTY, error output now shows a colored label alongside the JSON:
NO_COLORenv var (https://no-color.org/)std::io::IsTerminal+ ANSI escape codesChanges
src/error.rs: addstderr_supports_color(),colorize(),error_label()helpersprint_error_json(): print colored label to stderr before JSONNO_COLORand error label contentTest plan
cargo test— 586 passed, 0 failedcargo clippy -- -D warnings— cleanGenerated with Claude Code